ARROW-10927: [Rust][Parquet] Add Decimal to ArrayBuilderReader #8880

sweb · 2020-12-09T16:11:55Z

This PR introduces capabilities to construct a DecimalArray from Parquet files.

This is done by adding a new Converter<Vec<Option<ByteArray>>, DecimalArray> in parquet::arrow::converter:

pub struct DecimalArrayConverter {
    precision: i32,
    scale: i32,
}

It is then used in ArrayBuilderReader using a match guard to differentiate it from regular fixed size binaries:

  PhysicalType::FIXED_LEN_BYTE_ARRAY if cur_type.get_basic_info().logical_type() == LogicalType::DECIMAL =>

A test was added that uses a parquet file from PARQUET_TEST_DATA to check whether loading and casting to DecimalArray works. I did not find a corresponding json file with the correct values, but I used pyarrow to extract the correct values and hard coded them in the test.

I thought that this PR would require #8784 to be merged, but they are independent. I used the same JIRA issue here - I hope that this is okay.

codecov-io · 2020-12-09T16:15:54Z

Codecov Report

Merging #8880 (f2fc213) into master (d65ba4e) will decrease coverage by 0.00%.
The diff coverage is 75.40%.

@@            Coverage Diff             @@
##           master    #8880      +/-   ##
==========================================
- Coverage   83.25%   83.24%   -0.01%     
==========================================
  Files         196      196              
  Lines       48116    48177      +61     
==========================================
+ Hits        40059    40106      +47     
- Misses       8057     8071      +14

Impacted Files	Coverage Δ
rust/parquet/src/arrow/array_reader.rs	`77.00% <52.38%> (-0.56%)`	⬇️
rust/parquet/src/arrow/schema.rs	`91.31% <63.63%> (-0.50%)`	⬇️
rust/parquet/src/arrow/converter.rs	`68.68% <94.44%> (+5.72%)`	⬆️
rust/parquet/src/arrow/arrow_reader.rs	`91.16% <100.00%> (+0.57%)`	⬆️
rust/arrow/src/array/array_binary.rs	`90.73% <0.00%> (+0.21%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d65ba4e...56b643b. Read the comment docs.

github-actions · 2020-12-09T16:45:03Z

https://issues.apache.org/jira/browse/ARROW-10674

seddonm1 · 2020-12-09T21:19:03Z

Also https://issues.apache.org/jira/browse/ARROW-10818

sweb · 2020-12-10T12:21:53Z

Also https://issues.apache.org/jira/browse/ARROW-10818

@seddonm1 Thanks for mentioning this issue - I was not aware of it. Could you point me in the direction of some modules / tests I could use to get started with data fusion to help with it? Currently, DecimalArray is just a variant of FixedSizeBinaryArray that represents its value as i128 and I assume that ARROW-10818 requires some numeric operations on decimal values.

seddonm1 · 2020-12-10T20:02:57Z

The TPC-H queries are fairly basic but do decimal operations like these which I think are all decimal type:

                sum(l_quantity) as sum_qty,
                sum(l_extendedprice) as sum_base_price,
                sum(l_extendedprice * (1 - l_discount)) as sum_disc_price,
                sum(l_extendedprice * (1 - l_discount) * (1 + l_tax)) as sum_charge,
                avg(l_quantity) as avg_qty,
                avg(l_extendedprice) as avg_price,
                avg(l_discount) as avg_disc,

jorgecarleitao · 2020-12-11T04:24:10Z

@sweb , datafusion is built on top of the arrow crate, and relies on both the array types and the compute kernels.

Different nodes in the logical plan require different compute kernels. For example, cast, concat and take for sort, hashing for group-by ops with the type (if applicable), MutableArrayData for the join.

here is an example of a test that does not check IO with CSV, only the execute in memory.

I do not know if we have DecimalType on the files ARROW_TEST_DATA, but if yes, it is a matter of reading it in DataFusion and use it directly (and see where it breaks).

Essentially, almost everything that has a match datatype needs to be covered for a complete implementation.

sweb · 2020-12-11T06:57:47Z

@seddonm1 @jorgecarleitao thank you for providing context! I will have a look and try to come up with a first implementation.

nevi-me · 2020-12-15T13:16:31Z

I used the same JIRA issue here - I hope that this is okay.

Please open a separate JIRA 😃

The Parquet spec supports reading decimal data from a few other types (i32, i64, binary) https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#decimal.

I think reading from FixedSizeList is a good start, but we should tackle the other types at some point. Also see #8926 where I've made similar changes.

nevi-me

LGTM

sweb · 2020-12-15T17:35:03Z

I used the same JIRA issue here - I hope that this is okay.

Please open a separate JIRA 😃

The Parquet spec supports reading decimal data from a few other types (i32, i64, binary) https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#decimal.

I think reading from FixedSizeList is a good start, but we should tackle the other types at some point. Also see #8926 where I've made similar changes.

@nevi-me thank you for your review! I have opened a new issue with a subtask specific to this PR. I will try to provide implementations for missing types (I only considered fixed size binary, since this is what I get from Spark ;)) and add the corresponding writers as well.

github-actions · 2020-12-15T17:43:17Z

https://issues.apache.org/jira/browse/ARROW-10927

nevi-me · 2020-12-15T19:51:30Z

That's interesting. I tried reading a fixed size binary today (interval) but Spark complained that it doesn't support reading fixed size binary. So I thought decimal might be backed by something else.

seddonm1 · 2020-12-15T20:08:19Z

@nevi-me Here are the links to the Spark Parquet docs which talk about "legacy" mode which I think is what you are bumping into: https://spark.apache.org/docs/latest/sql-data-sources-parquet.html#configuration

sweb · 2020-12-16T13:45:34Z

Interesting - now I need to find out why we are writing files in legacy mode ;)

andygrove · 2020-12-16T15:43:33Z

Thanks @sweb. I would like to assign the JIRA to you but need to add you to the contributor role there first but I'm there are multiple JIRA accounts with your name. Could you let me know which one is yours?

sweb · 2020-12-16T16:09:01Z

Thanks @andygrove. florian.mueller is the correct one!

nevi-me · 2020-12-16T16:41:13Z

@andygrove @sweb we've got CI failures, I suspect we merged too many changes without rebasing.

andygrove · 2020-12-16T16:44:27Z

Thanks @nevi-me I am looking now

andygrove · 2020-12-16T16:45:08Z

I will revert this PR for now

nevi-me · 2020-12-16T16:49:56Z

The reason of the failure is not obvious to me. @sweb we'll need to rebase changes after we reopen this PR, so we can see what the issue is.

sweb · 2020-12-16T17:36:41Z

@nevi-me @andygrove I will take a look at it and try to resolve it once master is stable and this PR is reopened.

sweb · 2020-12-17T08:58:13Z

@nevi-me @andygrove I fixed the issue. There is still a CI failure, though I think that this is unrelated and just a hickup. Is there a way to restart the CI job without whitespace-commit?

nevi-me · 2020-12-17T09:11:09Z

I've restarted the job, we can merge this after CI is all green. Thanks @sweb

sweb marked this pull request as draft December 9, 2020 16:12

github-actions bot added Component: Rust Component: Parquet labels Dec 9, 2020

sweb changed the title ~~ARROW-10674: [Rust][Parquet] Add Decimal to ArrayBuilderReader WIP~~ ARROW-10674: [Rust][Parquet] Add Decimal to ArrayBuilderReader Dec 10, 2020

sweb marked this pull request as ready for review December 10, 2020 15:39

nevi-me requested review from nevi-me and sunchao December 15, 2020 13:09

nevi-me approved these changes Dec 15, 2020

View reviewed changes

sweb changed the title ~~ARROW-10674: [Rust][Parquet] Add Decimal to ArrayBuilderReader~~ ARROW-10927: [Rust][Parquet] Add Decimal to ArrayBuilderReader Dec 15, 2020

andygrove approved these changes Dec 16, 2020

View reviewed changes

andygrove closed this in 06ac750 Dec 16, 2020

nevi-me reopened this Dec 16, 2020

sweb added 2 commits December 17, 2020 07:40

feat: add parquet arrow reader for decimal

086d6d2

fix: avoid unnecessary cast

41fee88

sweb force-pushed the rust-parquet-decimal-arrow-reader branch from 84220b7 to 41fee88 Compare December 17, 2020 07:04

sweb added 2 commits December 17, 2020 08:05

fix: replace ByteArray with FixedLenByteArray

f2fc213

fix: formatting

56b643b

jorgecarleitao closed this in 519e9da Dec 18, 2020

This was referenced Dec 24, 2020

[Rust] Add integration tests for Decimal type #26627

Closed

[Rust] [Parquet] Add Decimal to ArrayBuilderReader for physical type fixed size binary #26855

Closed

ARROW-10927: [Rust][Parquet] Add Decimal to ArrayBuilderReader #8880

ARROW-10927: [Rust][Parquet] Add Decimal to ArrayBuilderReader #8880

Uh oh!

Conversation

sweb commented Dec 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Dec 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Dec 9, 2020

Uh oh!

seddonm1 commented Dec 9, 2020

Uh oh!

sweb commented Dec 10, 2020

Uh oh!

seddonm1 commented Dec 10, 2020

Uh oh!

jorgecarleitao commented Dec 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sweb commented Dec 11, 2020

Uh oh!

nevi-me commented Dec 15, 2020

Uh oh!

nevi-me left a comment

Choose a reason for hiding this comment

Uh oh!

sweb commented Dec 15, 2020

Uh oh!

github-actions bot commented Dec 15, 2020

Uh oh!

nevi-me commented Dec 15, 2020

Uh oh!

seddonm1 commented Dec 15, 2020

Uh oh!

sweb commented Dec 16, 2020

Uh oh!

andygrove commented Dec 16, 2020

Uh oh!

sweb commented Dec 16, 2020

Uh oh!

nevi-me commented Dec 16, 2020

Uh oh!

andygrove commented Dec 16, 2020

Uh oh!

andygrove commented Dec 16, 2020

Uh oh!

nevi-me commented Dec 16, 2020

Uh oh!

sweb commented Dec 16, 2020

Uh oh!

sweb commented Dec 17, 2020

Uh oh!

nevi-me commented Dec 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sweb commented Dec 9, 2020 •

edited

Loading

codecov-io commented Dec 9, 2020 •

edited

Loading

jorgecarleitao commented Dec 11, 2020 •

edited

Loading